Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Powerboost configuration #35

Merged
merged 4 commits into from
May 20, 2024
Merged

Powerboost configuration #35

merged 4 commits into from
May 20, 2024

Conversation

nanomad
Copy link
Contributor

@nanomad nanomad commented Apr 11, 2024

Thanks to @rubens73 for the powerboost implementation

This fixes #32

@tmenguy
Copy link

tmenguy commented Apr 11, 2024

Hi @nanomad , I took the liberty to add your schedule and ICP power in #34 .... as like you I was needing the schedule parts thanks a lot! So if you want to review what I did we could merge those 2 PR.

@nanomad
Copy link
Contributor Author

nanomad commented Apr 12, 2024

No worries :)

Closing this one instead

@nanomad nanomad closed this Apr 12, 2024
@tmenguy
Copy link

tmenguy commented Apr 12, 2024

As we are at it, what would be the best way to delete a schedule ?

@nanomad nanomad reopened this Apr 15, 2024
@nanomad
Copy link
Contributor Author

nanomad commented Apr 15, 2024

@cliviu74 Do you want me to split ICP and Schedule as well?

You can review this indepenedtly from the time-out so that @tmenguy can then rebase his own on top of this

@cliviu74
Copy link
Owner

@cliviu74 Do you want me to split ICP and Schedule as well?

You can review this indepenedtly from the time-out so that @tmenguy can then rebase his own on top of this

Hey, good catch, didn't even noticed there's another one in there.
Yes please, we need to have separate issues for each feature, ICP and Schedule, even if they seem related.

The ICP one could be an issue as early tests on chargers that don't have an energy meter installed throw some weird errors from the Wallbox API.

@nanomad nanomad changed the title Powerboost and schedule configuration Powerboost configuration Apr 15, 2024
@nanomad
Copy link
Contributor Author

nanomad commented Apr 15, 2024

@cliviu74 Split into #37

As far as I understand, we can probably feature wall this based on the product configuration API, altough I'd avoid having to call tow APIs just to update the ICP setting

@nanomad
Copy link
Contributor Author

nanomad commented Apr 15, 2024

Looking at the WB status endpoint, we can probably feature gate this by looking at:

  • status['config_data']['icp_max_current'] not None and > 0
  • 'POWER_BOOST' in status['config_data']['plan']['features']

@cliviu74
Copy link
Owner

Upon more testing, I found the following

  • on installations without a power diverter, this won't do anything. The upstream wallbox API will reply with a 400 message, other than that it seems pretty benign
  • I can't verify this indeed has the intended behaviour (i.e. setting ICP Max Current) so I'll take your word for it @nanomad
  • As this library is intended as a simple interface to the wallbox API, I think gating should be done upstream (i.e. Home Assistant integration?!)

Based on this, I'll merge as soon as comments on the code are fixed:

  • @nanomad can you please add documentation for the setIcpMaxCurrent method to README.md?

Copy link
Owner

@cliviu74 cliviu74 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, @nanomad can you please add the method description to README.md?!

@nanomad nanomad requested a review from cliviu74 May 20, 2024 09:24
@nanomad
Copy link
Contributor Author

nanomad commented May 20, 2024

@cliviu74 README updated

@cliviu74 cliviu74 merged commit 9feaa91 into cliviu74:master May 20, 2024
@rubenms73
Copy link

Thanks to @rubens73 for the powerboost implementation

This fixes #32

Hi @nanomad, my pleasure. My username is @rubenms73, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New feature addition request (code attached)
4 participants